feat: add executeJs helper #8
Conversation
javier-godoy
force-pushed
the
executejs
branch
from
ae26609 to
5a168a6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces a new public API for asynchronous JavaScript execution with JSON compatibility across Vaadin versions. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
154-164: Consider improving type safety in the wrap method.The current implementation uses a raw
SerializableConsumerreturn type and requires@SuppressWarningsannotations. This can be refactored for better type safety:Apply this diff to eliminate raw types:
- @SuppressWarnings("rawtypes") - private static SerializableConsumer wrap(SerializableConsumer<JsonValue> resultHandler) { - return (SerializableConsumer<JsonNode>) node -> resultHandler.accept(convertToJsonValue(node)); + private static SerializableConsumer<JsonNode> wrap(SerializableConsumer<JsonValue> resultHandler) { + return node -> resultHandler.accept(convertToJsonValue(node)); }; @Override - @SuppressWarnings("unchecked") public void then(SerializableConsumer<JsonValue> resultHandler, SerializableConsumer<String> errorHandler) { delegate.then(wrap(resultHandler), errorHandler);This makes the type conversion explicit and eliminates the need for suppression annotations.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
34-34: Add JavaDoc documentation for this public API method.This interface method lacks documentation for users to understand its purpose, parameters, return value, and behavior. Public API methods should include clear JavaDoc explaining the conversion and any null-handling behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(2 hunks)
🔇 Additional comments (6)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
119-133: LGTM! Well-documented public API addition.The
executeJsmethod properly delegates to Vaadin's native API and applies version-specific conversion through the helper. The javadoc clearly explains the purpose and references the underlying Vaadin API.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
142-146: LGTM! Proper wrapper instantiation for Vaadin 25+.The conversion method correctly instantiates the version-specific wrapper that handles JsonNode↔JsonValue conversion.
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java (2)
1-35: Proper attribution with dual copyright headers.The dual copyright (Flowing Code + Vaadin Ltd) is appropriate since this interface is derived from Vaadin's
PendingJavaScriptResultAPI. Both use Apache 2.0 licensing, ensuring compatibility.
42-89: LGTM! Well-designed compatibility interface.The interface provides a clean abstraction over
PendingJavaScriptResultusingelemental.json.JsonValueto maintain compatibility across Vaadin versions. The documentation is comprehensive, explaining async behavior and session locking constraints. The default convenience method properly delegates to the main method.src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
49-61: Elegant delegation pattern for legacy Vaadin versions.The implementation correctly wraps
PendingJavaScriptResultusing Lombok's@Delegateannotation. In Vaadin 24 and earlier,PendingJavaScriptResult.then()usesSerializableConsumer<JsonValue>, making this delegation approach work cleanly for pre-Vaadin 25 compatibility.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
22-22: LGTM!The import is necessary for the new method and correctly placed.
Add a helper for
executeJs.then(SerializableConsumer,...)(the original method receives aSerializableConsumer<JsonNode>in Vaadin 25, and aSerializableConsumer<JsonValue>in earlier versions).Summary by CodeRabbit